Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough조직(Organization)과 조직 템플릿(OrganizationTemplate) 도메인, JPA 엔티티, Spring Data 리포지토리, 도메인-리포지토리 적응층, DB 마이그레이션, 테스트 픽스처 및 단위 테스트를 추가합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ODR as OrganizationDomainRepository
participant OTR as OrganizationTemplateRepository
participant OR as OrganizationRepository
participant DB as Database
Client->>ODR: findAll()
ODR->>OTR: findAll()
OTR->>DB: SELECT * FROM organization_template
DB-->>OTR: List<OrganizationTemplateEntity>
OTR-->>ODR: List<OrganizationTemplateEntity>
Note over ODR: templates를 organizationId 기준으로 그룹화\n엔티티를 도메인으로 변환
ODR->>OR: findAll()
OR->>DB: SELECT * FROM organization
DB-->>OR: List<OrganizationEntity>
OR-->>ODR: List<OrganizationEntity>
Note over ODR: 각 OrganizationEntity에 매칭되는 templates 첨부\n도메인 Organization 목록 생성
ODR-->>Client: List<Organization>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @leegwichan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 기관(Organization)과 해당 기관에 속하는 템플릿(OrganizationTemplate)을 관리하기 위한 기본적인 데이터 모델과 영속성 계층을 구축합니다. Organization 및 OrganizationTemplate 엔티티와 도메인 객체를 정의하고, 데이터 접근을 위한 레포지토리를 구현하여 향후 기관별 템플릿 관리 기능 개발의 기반을 마련합니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
|
||
| return organizationRepository.findAll() | ||
| .stream() | ||
| .map(entity -> entity.toDomain(idToTemplatesEntity.get(entity.getId()))) |
There was a problem hiding this comment.
조직(Organization)에 연관된 템플릿이 없는 경우, idToTemplatesEntity.get(entity.getId())는 null을 반환할 수 있습니다. 이 null 값이 entity.toDomain() 메소드로 전달되면, 이후 getTemplates() 호출 시 NullPointerException이 발생할 수 있습니다. getOrDefault를 사용하여 템플릿이 없는 경우 빈 리스트를 기본값으로 제공하는 것이 안전합니다.
| .map(entity -> entity.toDomain(idToTemplatesEntity.get(entity.getId()))) | |
| .map(entity -> entity.toDomain(idToTemplatesEntity.getOrDefault(entity.getId(), List.of()))) |
| public class OrganizationDomainRepository { | ||
|
|
||
| private final OrganizationRepository organizationRepository; | ||
| private final OrganizationTemplateRepository organizationTemplateDomainRepository; |
There was a problem hiding this comment.
필드명 organizationTemplateDomainRepository는 혼동을 줄 수 있습니다. 이 클래스의 타입은 OrganizationTemplateRepository이며, 이는 일반적인 Spring Data Repository입니다. 이름에 'Domain'이 포함되어 있어 이 클래스(OrganizationDomainRepository)와 같은 커스텀 도메인 리포지토리로 오해할 수 있습니다. 명확성을 위해 필드명을 organizationTemplateRepository로 변경하는 것을 권장합니다.
| private final OrganizationTemplateRepository organizationTemplateDomainRepository; | |
| private final OrganizationTemplateRepository organizationTemplateRepository; |
There was a problem hiding this comment.
Github Copolit이 나빴네;;
미안합니다. 체크 못했습니다.
src/main/java/com/debatetimer/entity/organization/OrganizationTemplateEntity.java
Show resolved
Hide resolved
...est/java/com/debatetimer/domainrepository/organization/OrganizationDomainRepositoryTest.java
Show resolved
Hide resolved
Test Results126 files 126 suites 14s ⏱️ Results for commit 4e6b73a. ♻️ This comment has been updated with latest results. |
📝 Test Coverage Report
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/main/resources/db/migration/V15__create_organization_template.sql (2)
23-26: FK 제약조건에 ON DELETE 동작 명시를 고려하세요.현재 FK 제약조건에
ON DELETE절이 없어서 템플릿이 있는 Organization 삭제 시 참조 무결성 오류가 발생합니다. 의도된 동작이라면 괜찮지만, 향후 Organization 삭제 기능 구현 시ON DELETE CASCADE또는ON DELETE RESTRICT를 명시적으로 지정하는 것이 좋습니다.
12-21: organization_id 컬럼에 인덱스 추가를 고려하세요.
OrganizationDomainRepository.findAll()에서organization_id로 그룹핑하는 쿼리가 수행됩니다. 템플릿 수가 증가하면 인덱스가 조회 성능 향상에 도움이 될 수 있습니다.create table organization_template ( id bigint auto_increment, name varchar(255) not null, data varchar(8191) not null, organization_id bigint not null, created_at timestamp not null DEFAULT CURRENT_TIMESTAMP, modified_at timestamp not null DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - primary key (id) + primary key (id), + index idx_organization_template_organization_id (organization_id) );src/main/java/com/debatetimer/entity/organization/OrganizationTemplateEntity.java (1)
27-55: 엔티티–도메인 매핑 전반은 깔끔합니다만, 연관관계 null 제약을 JPA 레벨에도 명시하면 더 좋겠습니다
OrganizationTemplateEntity의 필드 구성과toDomain(),getOrganizationId()설계 모두 도메인 모델과 잘 맞습니다.
다만 조직 연관관계를 확실히 non‑null 로 강제하려면 JPA 매핑에도 제약을 명시하는 편이 의도가 더 분명해집니다.예를 들어 다음과 같이 수정하는 것을 고려해 볼 수 있습니다:
- @NotNull - @ManyToOne(fetch = FetchType.LAZY) - @JoinColumn(name = "organization_id") + @NotNull + @ManyToOne(fetch = FetchType.LAZY, optional = false) + @JoinColumn(name = "organization_id", nullable = false) private OrganizationEntity organization;이렇게 하면 Bean Validation (@NotNull) 뿐 아니라 JPA 매핑과 DDL 측에서도 일관되게 “항상 조직이 존재하는 템플릿”이라는 제약을 표현할 수 있습니다.
src/main/java/com/debatetimer/entity/organization/OrganizationEntity.java (1)
29-47:toDomain에서 템플릿 리스트를 null 대신 빈 리스트로 정규화하는 것이 안전합니다현재
OrganizationEntity.toDomain(List<OrganizationTemplate> templates)가 전달받은 리스트를 그대로 쓰고 있어, 호출 측(예:OrganizationDomainRepository)에서 매핑에 실패하면templates가 null 로 넘어올 수 있습니다. 이 경우 도메인Organization의templates필드도 null 이 되어 이후 사용 시 NPE 위험이 있습니다.다음처럼 null 을 빈 리스트로 정규화하면 도메인 계층에서 컬렉션을 항상 non‑null 로 다룰 수 있어 더 안전합니다.
- public Organization toDomain(List<OrganizationTemplate> templates) { - return new Organization(this.id, this.name, this.affiliation, this.iconPath, templates); - } + public Organization toDomain(List<OrganizationTemplate> templates) { + List<OrganizationTemplate> safeTemplates = (templates != null) ? templates : List.of(); + return new Organization(this.id, this.name, this.affiliation, this.iconPath, safeTemplates); + }추가로,
affiliation도 비어 있지 않은 문자열이어야 한다는 도메인 제약이라면@NotNull대신@NotBlank로 맞추는 것도 검토해볼 만합니다.src/main/java/com/debatetimer/domainrepository/organization/OrganizationDomainRepository.java (1)
21-35: 로직은 올바르지만 Repository/맵 변수 이름을 역할에 맞게 다듬으면 더 읽기 좋겠습니다
findAll()구현 자체는 의도대로 잘 동작합니다. 다만 필드·변수명이 실제 담고 있는 타입/역할과 약간 어긋나 가독성이 떨어질 수 있습니다.
organizationTemplateDomainRepository필드는 Spring DataOrganizationTemplateRepository타입이므로, 단순히organizationTemplateRepository가 더 직관적입니다.idToTemplatesEntity맵에는 이미OrganizationTemplateEntity::toDomain결과(도메인 객체)들이 들어가므로,idToTemplates정도의 이름이 더 정확합니다.예를 들면 다음과 같이 정리할 수 있습니다:
- private final OrganizationTemplateRepository organizationTemplateDomainRepository; + private final OrganizationTemplateRepository organizationTemplateRepository; @@ - Map<Long, List<OrganizationTemplate>> idToTemplatesEntity = organizationTemplateDomainRepository.findAll() + Map<Long, List<OrganizationTemplate>> idToTemplates = organizationTemplateRepository.findAll() .stream() .collect(groupingBy( OrganizationTemplateEntity::getOrganizationId, - mapping(OrganizationTemplateEntity::toDomain, toList())) - ); + mapping(OrganizationTemplateEntity::toDomain, toList()) + )); @@ - return organizationRepository.findAll() - .stream() - .map(entity -> entity.toDomain(idToTemplatesEntity.get(entity.getId()))) - .toList(); + return organizationRepository.findAll() + .stream() + .map(entity -> entity.toDomain(idToTemplates.get(entity.getId()))) + .toList();
OrganizationEntity.toDomain()에서 null 방어를 추가한다면, 이 메서드는 현재 형태 그대로도 충분히 견고하게 동작할 것 같습니다.src/test/java/com/debatetimer/fixture/entity/OrganizationTemplateEntityGenerator.java (1)
11-23: 테스트용 템플릿 생성기가 단순·명확하고, 재사용성도 좋습니다
DEFAULT_TEMPLATE_CONTENT를 상수로 두고generate(organization, name)에서OrganizationTemplateEntity를 생성·저장하는 흐름이 깔끔합니다. 도메인 리포지토리 테스트에서 읽기 좋은 픽스처 구성이라고 생각됩니다.다만, 이 긴 문자열이 압축/인코딩된 특정 JSON 이나 템플릿이라면, 나중에 유지보수를 위해 정체를 짧게 주석으로 남기거나 별도 리소스/헬퍼 클래스로 분리해 두는 것도 고려해볼 수 있습니다 (필수는 아니고 선택 사항입니다).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/com/debatetimer/domain/organization/Organization.java(1 hunks)src/main/java/com/debatetimer/domain/organization/OrganizationTemplate.java(1 hunks)src/main/java/com/debatetimer/domainrepository/organization/OrganizationDomainRepository.java(1 hunks)src/main/java/com/debatetimer/entity/organization/OrganizationEntity.java(1 hunks)src/main/java/com/debatetimer/entity/organization/OrganizationTemplateEntity.java(1 hunks)src/main/java/com/debatetimer/repository/organization/OrganizationRepository.java(1 hunks)src/main/java/com/debatetimer/repository/organization/OrganizationTemplateRepository.java(1 hunks)src/main/resources/db/migration/V15__create_organization_template.sql(1 hunks)src/test/java/com/debatetimer/domainrepository/BaseDomainRepositoryTest.java(2 hunks)src/test/java/com/debatetimer/domainrepository/organization/OrganizationDomainRepositoryTest.java(1 hunks)src/test/java/com/debatetimer/fixture/entity/OrganizationEntityGenerator.java(1 hunks)src/test/java/com/debatetimer/fixture/entity/OrganizationTemplateEntityGenerator.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/main/java/com/debatetimer/repository/organization/OrganizationRepository.java (1)
src/main/java/com/debatetimer/domainrepository/organization/OrganizationDomainRepository.java (1)
Repository(17-37)
src/main/java/com/debatetimer/repository/organization/OrganizationTemplateRepository.java (1)
src/main/java/com/debatetimer/domainrepository/organization/OrganizationDomainRepository.java (1)
Repository(17-37)
src/main/java/com/debatetimer/entity/organization/OrganizationEntity.java (3)
src/main/java/com/debatetimer/entity/organization/OrganizationTemplateEntity.java (1)
Table(21-56)src/main/java/com/debatetimer/domain/organization/Organization.java (1)
Getter(6-26)src/main/java/com/debatetimer/domain/organization/OrganizationTemplate.java (1)
Getter(5-17)
src/test/java/com/debatetimer/fixture/entity/OrganizationEntityGenerator.java (1)
src/test/java/com/debatetimer/fixture/entity/OrganizationTemplateEntityGenerator.java (1)
Component(8-24)
src/main/java/com/debatetimer/domain/organization/Organization.java (1)
src/main/java/com/debatetimer/domain/organization/OrganizationTemplate.java (1)
Getter(5-17)
src/main/java/com/debatetimer/entity/organization/OrganizationTemplateEntity.java (3)
src/main/java/com/debatetimer/entity/organization/OrganizationEntity.java (1)
Table(19-48)src/main/java/com/debatetimer/domain/organization/Organization.java (1)
Getter(6-26)src/main/java/com/debatetimer/domain/organization/OrganizationTemplate.java (1)
Getter(5-17)
src/main/java/com/debatetimer/domain/organization/OrganizationTemplate.java (1)
src/main/java/com/debatetimer/domain/organization/Organization.java (1)
Getter(6-26)
src/test/java/com/debatetimer/fixture/entity/OrganizationTemplateEntityGenerator.java (1)
src/test/java/com/debatetimer/fixture/entity/OrganizationEntityGenerator.java (1)
Component(7-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (5)
src/main/java/com/debatetimer/domain/organization/OrganizationTemplate.java (1)
5-17: LGTM!불변 도메인 객체로 잘 구현되었습니다.
@Getter와final필드를 사용하여 불변성을 보장합니다.src/main/java/com/debatetimer/repository/organization/OrganizationTemplateRepository.java (1)
7-12: LGTM!최소한의 인터페이스 설계가 잘 적용되었습니다.
JpaRepository대신 기본Repository를 확장하여 필요한 메서드만 노출하는 방식이 좋습니다.src/test/java/com/debatetimer/fixture/entity/OrganizationEntityGenerator.java (1)
7-22: LGTM!테스트 픽스처가
OrganizationTemplateEntityGenerator와 일관된 패턴으로 잘 구현되었습니다. 생성자 주입과 기본값 상수 사용이 적절합니다.src/main/java/com/debatetimer/repository/organization/OrganizationRepository.java (1)
7-12: LGTM!
OrganizationTemplateRepository와 일관된 설계입니다. 최소 인터페이스 원칙을 잘 따르고 있습니다.src/test/java/com/debatetimer/domainrepository/BaseDomainRepositoryTest.java (1)
11-12: 새 Organization 픽스처 주입이 기존 패턴과 잘 맞습니다
OrganizationEntityGenerator,OrganizationTemplateEntityGenerator를 BaseDomainRepositoryTest 에서 주입하는 방식이 기존 다른 Generator 들(BellEntityGenerator,Customize*Generator등)과 동일한 패턴이라 일관성이 좋습니다. 별도의 수정이 필요한 부분은 없어 보입니다.Also applies to: 54-58
| assertAll( | ||
| () -> assertThat(organizations).hasSize(2), | ||
| () -> assertThat(organizations.get(0).getTemplates()).hasSize(2), | ||
| () -> assertThat(organizations.get(1).getTemplates()).hasSize(1) | ||
| ); |
There was a problem hiding this comment.
테스트 결과 검증이 순서에 의존적입니다.
organizations.get(0)과 organizations.get(1)의 순서가 보장되지 않을 수 있습니다. 데이터베이스 또는 쿼리 구현에 따라 순서가 달라질 수 있어 테스트가 불안정해질 수 있습니다.
순서에 의존하지 않는 검증으로 개선하는 것을 권장합니다:
assertAll(
() -> assertThat(organizations).hasSize(2),
- () -> assertThat(organizations.get(0).getTemplates()).hasSize(2),
- () -> assertThat(organizations.get(1).getTemplates()).hasSize(1)
+ () -> assertThat(organizations)
+ .extracting(org -> org.getTemplates().size())
+ .containsExactlyInAnyOrder(2, 1)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assertAll( | |
| () -> assertThat(organizations).hasSize(2), | |
| () -> assertThat(organizations.get(0).getTemplates()).hasSize(2), | |
| () -> assertThat(organizations.get(1).getTemplates()).hasSize(1) | |
| ); | |
| assertAll( | |
| () -> assertThat(organizations).hasSize(2), | |
| () -> assertThat(organizations) | |
| .extracting(org -> org.getTemplates().size()) | |
| .containsExactlyInAnyOrder(2, 1) | |
| ); |
🤖 Prompt for AI Agents
In
src/test/java/com/debatetimer/domainrepository/organization/OrganizationDomainRepositoryTest.java
around lines 32-36, the assertions rely on organizations.get(0)/get(1) order
which may be non-deterministic; make the test order-independent by either
sorting the organizations list by a stable key (e.g., id or name) before
asserting template sizes, or by asserting on the multiset of template counts
(extract template sizes and assert it contains 2 and 1 in any order) so the test
no longer depends on element order.
unifolio0
left a comment
There was a problem hiding this comment.
/noti
@leegwichan 리뷰 남겼습니다~
| public class OrganizationDomainRepository { | ||
|
|
||
| private final OrganizationRepository organizationRepository; | ||
| private final OrganizationTemplateRepository organizationTemplateDomainRepository; |
|
|
||
| return organizationRepository.findAll() | ||
| .stream() | ||
| .map(entity -> entity.toDomain(idToTemplatesEntity.get(entity.getId()))) |
| @Column(name = "icon_path") | ||
| @NotBlank | ||
| private String iconPath; |
There was a problem hiding this comment.
다른 컬럼과 달리 여기선 name을 명시해준 이유가 있나요?
| @NotBlank | ||
| @Column(length = 8191) | ||
| private String data; |
There was a problem hiding this comment.
제가 생각했던 방식은 서버에서 단순히 인코딩된 정보를 저장하고 반환만 하는 것이 아닌 인코딩/디코딩도 서버로 가져오고 테이블을 DB에 저장하는 것이였는데 나중에 토의해보면 좋을 것 같아요
There was a problem hiding this comment.
처음 테크독 작성할 때 피드백했으면 반영을 했을수도 있는데, 그렇지 않았어요.
그 때 당시의 나는 굉장히 테스크가 많았기 때문에 이 방법이 최선이었어요.
이걸 옮기는 것은 내가 여유있을 때 진행하도록 합니다. (YAPP 제발 끝나라... 제발...)
|
|
||
| private final Long id; | ||
| private final String name; | ||
| private final String data; |
There was a problem hiding this comment.
굳이 싶긴 하지만 확인해보니 Test에서도 실제 json을 인코딩한 값을 넣어주고 있던데 그러면 여기서 디코딩해서 json형식인지 검증하는 것도 괜찮아 보이네요
There was a problem hiding this comment.
이게 인코딩/디코딩 방식이 '특정 인코딩 방식' <-> BASE 64 <-> JSON 형식으로 이중으로 되어있어요. (BASE64로만 하면 너무 길다고 해서;;)
그리고 해당 로직은 기본적으로 조회 기능만 있기 때문에 아직은 필요없다고 생각했습니다.
leegwichan
left a comment
There was a problem hiding this comment.
/noti @unifolio0
반영 완료했습니다. 쉬엄쉬엄 리뷰해주세요~
|
|
||
| private final Long id; | ||
| private final String name; | ||
| private final String data; |
There was a problem hiding this comment.
이게 인코딩/디코딩 방식이 '특정 인코딩 방식' <-> BASE 64 <-> JSON 형식으로 이중으로 되어있어요. (BASE64로만 하면 너무 길다고 해서;;)
그리고 해당 로직은 기본적으로 조회 기능만 있기 때문에 아직은 필요없다고 생각했습니다.
| public class OrganizationDomainRepository { | ||
|
|
||
| private final OrganizationRepository organizationRepository; | ||
| private final OrganizationTemplateRepository organizationTemplateDomainRepository; |
There was a problem hiding this comment.
Github Copolit이 나빴네;;
미안합니다. 체크 못했습니다.
|
|
||
| return organizationRepository.findAll() | ||
| .stream() | ||
| .map(entity -> entity.toDomain(idToTemplatesEntity.get(entity.getId()))) |
| @Column(name = "icon_path") | ||
| @NotBlank | ||
| private String iconPath; |
| @NotBlank | ||
| @Column(length = 8191) | ||
| private String data; |
There was a problem hiding this comment.
처음 테크독 작성할 때 피드백했으면 반영을 했을수도 있는데, 그렇지 않았어요.
그 때 당시의 나는 굉장히 테스크가 많았기 때문에 이 방법이 최선이었어요.
이걸 옮기는 것은 내가 여유있을 때 진행하도록 합니다. (YAPP 제발 끝나라... 제발...)
coli-geonwoo
left a comment
There was a problem hiding this comment.
/noti
커찬, YAPP 과 병행하느라 고생이 많습니다.. 몇가지 리뷰 남겼어요!
| import lombok.Getter; | ||
|
|
||
| @Getter | ||
| public class Organization { |
There was a problem hiding this comment.
혹시, Lombok 안쓴 이유가 따로 있나요?
| public class Organization { | |
| @RequiresArgsConstructor | |
| public class Organization { |
| this.id = id; | ||
| this.name = name; | ||
| this.affiliation = affiliation; | ||
| this.iconPath = iconPath; |
There was a problem hiding this comment.
단순 궁금증인데 iconPath 보면 아이콘들을 서버에서 관리할 것 같은데 대략적으로 어떻게 관리할 예정인가요?
leegwichan
left a comment
There was a problem hiding this comment.
/noti @coli-geonwoo
리뷰 코멘트 달았습니다. 머지하도록 할께요.
| this.id = id; | ||
| this.name = name; | ||
| this.affiliation = affiliation; | ||
| this.iconPath = iconPath; |
| import lombok.Getter; | ||
|
|
||
| @Getter | ||
| public class Organization { |



🚩 연관 이슈
#222
PR 소개
구현 사항
추후 구현 사항
관련 문서 및 기타
Summary by CodeRabbit
릴리스 노트
새 기능
데이터베이스
테스트
✏️ Tip: You can customize this high-level summary in your review settings.